Make percentage string values as floats/ints in InfluxDB by philhawthorne · Pull Request #7879 · home-assistant/core

philhawthorne · 2017-06-03T06:51:11Z

Description:

Currently Z-wave and other components report an attributes battery
level as an integer, for example

{
"is_awake": false,
"battery_level": 61,
}

However, some other components like Vera add the battery level as a
string

{
"Vera Device Id": 25,
"device_armed": "False",
"battery_level": "63%",
"device_tripped": "False",
}

By removing any % signs in the field, this will send the value to
InfluxDB as an int, which can then be used to plot the data in graphs
correctly, like other percentage fields.

This PR will detect when a percentage is being cast as a string, and remove
the % sign before it is shipped over to InfluxDB.

Not really a Python dev, so if there's a better way to do this, or some tests I
need to add any help would be appreciated.

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

Currently Z-wave and other compontents report an attributes battery level as an integer, for example ```yaml { "is_awake": false, "battery_level": 61, } ``` However, some other components like Vera add the battery level as a string ```yaml { "Vera Device Id": 25, "device_armed": "False", "battery_level": "63%", "device_tripped": "False", } ``` By removing any % signs in the field, this will send the value to InfluxDB as an int, which can then be used to plot the data in graphs correctly, like other percentage fields.

mention-bot · 2017-06-03T06:51:12Z

@philhawthorne, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @balloob and @titilambert to be potential reviewers.

lwis · 2017-06-03T10:06:54Z

There's a test for InfluxDB, could you update that also, please?

MartinHjelmare · 2017-06-03T10:38:06Z

Is it the correct approach to do this parsing in Influxdb? Maybe each component or base component should make sure not to include units in the values reported?

lwis · 2017-06-03T10:42:51Z

That only really works for state and unit_of_measurement. But yes, I agree that attributes should not contain units - but there's currently no uniform way to store units for attributes.

Would be good to have something like a dict with a unit, however this would be quite a big change as attributes are just key with simple value at the moment.

MartinHjelmare · 2017-06-03T11:22:49Z

Yes, it wouldn't be a quick fix.

If we add the fix here, can we make it broader somehow, to not only target %? Maybe:

Strip all non alfanumeric characters.
Check if only numeric characters remain.
If so: return those (including decimal point), if not: return the original

philhawthorne · 2017-06-03T12:32:19Z

@MartinHjelmare

I thought about that. I was worried this would happen

{
"friendly_name": "Living Lamp 1"
},
{
"friendly_name": "Moto 2X9P"
}

Would just become

{
"friendly_name": 1
},
{
"friendly_name": 29
}

I guess we could keep storing the _str version as well.

{
"friendly_name": 29,
"friendly_name_str": "Moto 2X9P"
}

But not sure if adding so many extra keys would be a good idea?

lwis · 2017-06-03T12:35:11Z

@philhawthorne I think we should only strip trailing characters, and also store the original string 👍

philhawthorne · 2017-06-03T12:37:58Z

@lwis yep that makes sense! I'll make the changes and update the tests to check for it as well

Adds tests and now removes all trailing non-numeric characters for better use

philhawthorne · 2017-06-05T12:06:45Z

Added tests and now removes all trailing non-digits from the attribute.

For example

{
"battery_level": "44%",
"friendly_name": "Johns iPhone 2"
}

Will become

{
"battery_level_str": "44%",
"battery_level": 44,
"friendly_name_str": "Johns iPhone 2"
}

MartinHjelmare · 2017-06-05T12:09:47Z

        ]

-        percent_remove_regex = re.compile(r"^([0-9.])+\%$")
+        percent_remove_regex = re.compile(r'[\d.]+')


You should call this something else now, right? non_digit_tail?

Correct! Cheers. Have updated that

Updates the variable used for the regex to remove trailing non digits

lwis · 2017-06-07T11:49:40Z

I'm happy with this now, it seems like a sensible change. @MartinHjelmare do you have anything else, or can this be merged?

MartinHjelmare · 2017-06-07T17:15:42Z

If the regex works I'm happy. There's a small lint error that needs fixing before we can merge.

pvizeli

think it is fine to cast the type for influxdb. But we should also fix the Vera component/platform. The state should not be include any types, that is a bug of device and need to be fix.

In this case, remove the *_str. That should be the same as the casted type. All other is a bug that need fix on right position and not inside influxdb. But with this new cast it will make that component more robust.

Fixes a small linting error on the InfluxDB component

philhawthorne · 2017-06-09T03:44:22Z

@pvizeli should we update the Vera plugin in this Pull Request? This PR will cover all components that may have this problem.

Wondering if it would be better to separate the Vera updates into a different PR as it may be a breaking change?

pvizeli · 2017-06-13T22:42:50Z

Yes, the fix on other component/platform should be done in a new PR. I see that the *_str are not a part of this PR. I think we should be able now to remove this *_str in a new PR after we merge it. With this change we need not do that anymore.

diplix · 2017-06-18T21:33:32Z

this change produces many new error for entities that have IP-addresses as attribute, because the regex does not consider strings with more than one dot as not convertible to floats. it seems to just tries to convert everything with a dot in it to a float.

ERROR:homeassistant.core:Error doing job: Future exception was never retrieved
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/homeassistant/components/influxdb.py", line 164, in influx_event_listener
    json_body[0]['fields'][key] = float(value)
ValueError: could not convert string to float: '192.168.1.69'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/homeassistant/components/influxdb.py", line 170, in influx_event_listener
    non_decimal.sub('', value))
ValueError: could not convert string to float: '192.168.1.69'

homeassistant added integration: influxdb cla-signed labels Jun 3, 2017

balloobbot added the component label Jun 3, 2017

Add tests and remove all trailing non digits

7e18ada

Adds tests and now removes all trailing non-numeric characters for better use

homeassistant added the cla-signed label Jun 5, 2017

MartinHjelmare requested changes Jun 5, 2017

View reviewed changes

Update variable name for InfluxDB digit checks

56e6ca1

Updates the variable used for the regex to remove trailing non digits

MartinHjelmare approved these changes Jun 7, 2017

View reviewed changes

pvizeli requested changes Jun 8, 2017

View reviewed changes

Fix linting errors for InfluxDB component

b73274b

Fixes a small linting error on the InfluxDB component

homeassistant added the cla-signed label Jun 9, 2017

pvizeli approved these changes Jun 13, 2017

View reviewed changes

pvizeli merged commit 1ddcab5 into home-assistant:dev Jun 13, 2017

mezz64 mentioned this pull request Jun 15, 2017

InfluxDB datetime to float TypeError after #7879 #8042

Closed

balloob mentioned this pull request Jun 16, 2017

0.47 #8055

Merged

philhawthorne mentioned this pull request Jun 17, 2017

Remove % sign from Vera Battery Levels #8069

Merged

1 task

azogue mentioned this pull request Jun 18, 2017

influxdb errors when trying to parse some state attributes #8086

Closed

home-assistant locked and limited conversation to collaborators Oct 20, 2017

Uh oh!

Conversation

philhawthorne commented Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

mention-bot commented Jun 3, 2017

Uh oh!

lwis commented Jun 3, 2017

Uh oh!

MartinHjelmare commented Jun 3, 2017

Uh oh!

lwis commented Jun 3, 2017

Uh oh!

MartinHjelmare commented Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philhawthorne commented Jun 3, 2017

Uh oh!

lwis commented Jun 3, 2017

Uh oh!

philhawthorne commented Jun 3, 2017

Uh oh!

philhawthorne commented Jun 5, 2017

Uh oh!

MartinHjelmare Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philhawthorne Jun 5, 2017

Choose a reason for hiding this comment

Uh oh!

lwis commented Jun 7, 2017

Uh oh!

MartinHjelmare commented Jun 7, 2017

Uh oh!

pvizeli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philhawthorne commented Jun 9, 2017

Uh oh!

pvizeli commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diplix commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

philhawthorne commented Jun 3, 2017 •

edited

Loading

MartinHjelmare commented Jun 3, 2017 •

edited

Loading

MartinHjelmare Jun 5, 2017 •

edited

Loading

pvizeli left a comment •

edited

Loading

pvizeli commented Jun 13, 2017 •

edited

Loading

diplix commented Jun 18, 2017 •

edited

Loading